feat: add branching #195
feat: add branching #195
Conversation
WalkthroughAdd optional branch support across schema, config, CLI options, commands, utilities, and tests; introduce branch and merge CLI commands; propagate branch context into API calls and messages; add HTTP 404 handling in errorFromLoadable; minor .npmrc and HACKING.md tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI
participant Config as "Local Config"
participant Command as "branch/merge/pull/push/sync"
participant API as "Tolgee API"
participant FS as "Local FS / Extractor"
Note over CLI,Config: User runs CLI (may pass -b/--branch) or rely on config.branch
User->>CLI: invoke command (with optional --branch)
CLI->>Config: load config (reads config.branch)
CLI->>Command: start command (opts.branch resolved)
Command->>API: request (includes branch in query/body)
API-->>Command: response
alt Fetch & extract flow
Command->>FS: extract/unzip (log includes appendBranch)
FS-->>Command: done
end
Command-->>CLI: complete (messages include appendBranch)
CLI-->>User: output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (3)src/commands/tag.ts (1)
src/commands/sync/compare.ts (2)
src/commands/sync/sync.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils/branch.ts (1)
1-3: Consider clarifying the no-branch message.The message
" (no or default branch)"is ambiguous—it's unclear whether the system is using a default branch or operating without any branch context. Consider making the message more explicit about the actual behavior.💡 Possible alternative wording
If no branch means operating on the default/main branch:
- return branch ? ` (branch "${branch}")` : ' (no or default branch)'; + return branch ? ` (branch "${branch}")` : ' (default branch)';Or if it means no branch filtering is applied:
- return branch ? ` (branch "${branch}")` : ' (no or default branch)'; + return branch ? ` (branch "${branch}")` : ' (all branches)';Choose based on the actual Tolgee API behavior when no branch is specified.
src/commands/push.ts (1)
247-247: Consider adjusting the message when no branch is specified.When
opts.branchis undefined,appendBranchreturns' (no or default branch)', which will display as"Importing (no or default branch)...". This might be confusing for users who don't use branching at all. Consider showing the branch suffix only when a branch is explicitly specified:🔎 Suggested change to appendBranch utility
// In src/utils/branch.ts export function appendBranch(branch?: string) { - return branch ? ` (branch "${branch}")` : ' (no or default branch)'; + return branch ? ` (branch "${branch}")` : ''; }Alternatively, if the current behavior is intentional to always indicate branch status, the message could be rephrased to
' (default branch)'for clarity.Also applies to: 263-263
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.npmrc(0 hunks)HACKING.md(1 hunks)schema.json(1 hunks)src/cli.ts(2 hunks)src/commands/pull.ts(3 hunks)src/commands/push.ts(4 hunks)src/config/tolgeerc.ts(2 hunks)src/options.ts(2 hunks)src/schema.d.ts(1 hunks)src/utils/branch.ts(1 hunks)test/__fixtures__/validTolgeeRc/withBranch.json(1 hunks)test/unit/config.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .npmrc
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T09:27:51.189Z
Learnt from: stepan662
Repo: tolgee/tolgee-cli PR: 185
File: src/schema.d.ts:150-152
Timestamp: 2025-08-13T09:27:51.189Z
Learning: In the Tolgee CLI codebase, schema validation uses JSON schema from schema.json as the source of truth. The src/schema.d.ts file contains automatically generated TypeScript types derived from the JSON schema for compile-time type checking only, not runtime exports. CLI validation happens through JSON schema validation, not through runtime TypeScript enums.
Applied to files:
src/schema.d.tssrc/config/tolgeerc.ts
🧬 Code graph analysis (4)
test/unit/config.test.ts (1)
src/config/tolgeerc.ts (1)
loadTolgeeRc(102-133)
src/cli.ts (1)
src/options.ts (1)
PROJECT_BRANCH(59-62)
src/commands/pull.ts (4)
src/utils/branch.ts (1)
appendBranch(1-3)src/utils/prepareDir.ts (1)
prepareDir(4-15)src/utils/logger.ts (1)
loading(125-155)src/utils/zip.ts (1)
unzipBuffer(32-45)
src/commands/push.ts (1)
src/utils/branch.ts (1)
appendBranch(1-3)
🔇 Additional comments (16)
HACKING.md (1)
17-18: LGTM: Grammar improvement.The addition of "to" improves the grammatical clarity of the sentence.
src/schema.d.ts (1)
83-86: LGTM: Type definition aligns with schema.The optional branch field is correctly defined and consistent with the schema.json update in this PR.
test/__fixtures__/validTolgeeRc/withBranch.json (1)
1-5: LGTM: Valid test fixture.The fixture appropriately tests configuration loading with a branch value.
test/unit/config.test.ts (1)
97-105: LGTM: Test case properly validates branch loading.The test follows established patterns and correctly verifies that branch values are loaded from configuration files.
src/commands/pull.ts (3)
13-13: LGTM: Proper import of branch utility.
45-45: LGTM: Branch filtering correctly integrated.The branch value is properly passed to the export API call via
filterBranch, enabling branch-aware data fetching.
65-72: LGTM: User messages enhanced with branch context.Both fetch and extract operations now provide clear branch context to users through the
appendBranchutility.src/cli.ts (2)
22-22: LGTM: Proper import of branch option.
184-184: LGTM: Branch option correctly wired with config default.The PROJECT_BRANCH option follows the established pattern for other global options, properly defaulting to the value from configuration.
schema.json (1)
8-12: Branch whitespace validation is properly handled in the config parser.The schema correctly enforces
minLength: 1for the branch property. The config parser atsrc/config/tolgeerc.ts(lines 42-49) validates that whitespace-only strings are rejected by checking!rc.branch.trim().lengthbefore accepting the value, and then trims the branch accordingly.src/commands/push.ts (2)
32-32: LGTM!Import is correctly added for the new branch utility function.
225-225: LGTM!Branch parameter is correctly included in the import request payload, consistent with the new branching feature.
src/options.ts (2)
36-36: LGTM!The optional
branchfield is correctly added toBaseOptions, consistent with other optional fields in the interface.
59-62: LGTM!The new
PROJECT_BRANCHoption follows the established patterns for other CLI options:
- Uses consistent flag naming (
-b, --branch)- Environment variable support via
.env('TOLGEE_BRANCH')- Clear description indicating when to use the option
src/config/tolgeerc.ts (2)
1-12: LGTM!Good modernization of imports:
- Using
node:prefixes for Node.js built-in modules is the recommended ESM convention- Type-only imports for
CosmiconfigResultandSchemaare appropriate since they're only used for type annotations
42-49: LGTM!The branch validation logic is well-implemented:
- Correctly handles undefined (skips validation)
- Validates non-empty string requirement
- Properly trims whitespace and stores the normalized value
- Error message is consistent with the existing validation style
The defensive
typeof rc.branch !== 'string'check provides good runtime safety for user-provided config values, which is appropriate given configs are loaded dynamically. Based on learnings, this runtime validation complements the JSON schema validation that runs afterwards.
zomp
force-pushed
the
janmolnar/branching
branch
from
5fc1647 to
fd4c053
Compare
There is `enable-pre-post-scripts` option which is set to true and is true by default, but it throws a warning, so I am removing it: npm warn Unknown project config "enable-pre-post-scripts". This will stop working in the next major version of npm.
zomp
force-pushed
the
janmolnar/branching
branch
from
fd4c053 to
5a93411
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.npmrcHACKING.mdschema.jsonsrc/cli.tssrc/client/errorFromLoadable.tssrc/client/internal/schema.generated.tssrc/commands/pull.tssrc/commands/push.tssrc/commands/sync/compare.tssrc/config/tolgeerc.tssrc/options.tssrc/schema.d.tssrc/utils/branch.tstest/__fixtures__/validTolgeeRc/withBranch.jsontest/e2e/compare.test.tstest/e2e/utils/api/common.tstest/unit/config.test.ts
💤 Files with no reviewable changes (1)
- .npmrc
🚧 Files skipped from review as they are similar to previous changes (12)
- src/commands/sync/compare.ts
- src/cli.ts
- src/options.ts
- src/config/tolgeerc.ts
- schema.json
- src/utils/branch.ts
- test/fixtures/validTolgeeRc/withBranch.json
- src/commands/pull.ts
- src/commands/push.ts
- test/unit/config.test.ts
- HACKING.md
- src/schema.d.ts
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/compare.test.ts (3)
test/e2e/utils/api/common.ts (4)
createProjectWithClient(49-94)createBranch(100-126)createKey(132-146)deleteProject(39-43)test/e2e/utils/api/project2.ts (1)
PROJECT_2(6-15)test/e2e/utils/run.ts (1)
run(106-112)
test/e2e/utils/api/common.ts (1)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
🔇 Additional comments (7)
src/client/errorFromLoadable.ts (1)
46-49: LGTM! Clean addition of 404 error handling.The new case for HTTP 404 follows the established pattern and provides a clear, actionable error message. The inline comment helpfully contextualizes the scenario (missing branch), which aligns well with the PR's branching feature.
test/e2e/compare.test.ts (4)
6-7: LGTM!The new imports are properly used in the branching test suite.
39-39: LGTM!Good consistency improvement to match the PROJECT_2 constant being used.
71-72: LGTM!The updated assertions with
+/-prefixes improve clarity and align with the enhanced output format of the compare command.Also applies to: 86-87, 111-116, 135-140
160-230: Well-structured branching test suite.The tests comprehensively cover:
- Keys existing in the feature branch
- Keys not existing in the main branch
- Error handling for non-existent branches
The setup and teardown logic is appropriate.
test/e2e/utils/api/common.ts (2)
56-58: LGTM!Adding the
filterCurrentUserOwnerfilter improves test reliability by ensuring the correct organization is selected.
128-146: LGTM!Clean and type-safe implementation with appropriate defaults and option handling.
zomp
force-pushed
the
janmolnar/branching
branch
from
5a93411 to
d8da832
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/e2e/compare.test.ts (1)
212-229: Consider asserting a more specific error code or message.The test correctly verifies that referencing a non-existent branch fails. However, checking
out.stderr + out.stdoutis a bit broad. If the CLI has a consistent error output location, consider checking only that stream. Also, verifying the exit code is non-zero is good, but you might want to assert a specific exit code if the CLI uses distinct codes for different error types.test/e2e/utils/api/common.ts (1)
96-125: Consider returning the created branch for potential future use.The function works correctly for current test needs. However, returning the API response would allow callers to access the created branch ID if needed later.
🔎 Proposed enhancement
export async function createBranch( client: TolgeeClient, name: string, options?: CreateBranchOptions ) { let originBranchId = options?.originBranchId; if (!originBranchId) { const branches = await client.GET('/v2/projects/{projectId}/branches', { params: { path: { projectId: client.getProjectId() }, }, }); const origin = branches.data?._embedded?.branches?.find((b) => b.isDefault); originBranchId = origin?.id; } if (!originBranchId) { throw new Error('Default branch not found'); } - await client.POST('/v2/projects/{projectId}/branches', { + return await client.POST('/v2/projects/{projectId}/branches', { params: { path: { projectId: client.getProjectId() } }, body: { name, originBranchId }, }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/errorFromLoadable.tssrc/utils/branch.tstest/e2e/compare.test.tstest/e2e/utils/api/common.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/branch.ts
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/utils/api/common.ts (2)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)src/client/internal/schema.generated.ts (1)
components(5440-9752)
test/e2e/compare.test.ts (2)
test/e2e/utils/api/common.ts (4)
createProjectWithClient(49-94)createPak(159-165)createBranch(100-125)createKey(131-145)test/e2e/utils/api/project2.ts (1)
PROJECT_2(6-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
🔇 Additional comments (10)
src/client/errorFromLoadable.ts (1)
46-48: LGTM! Clean addition of 404 error handling.The implementation correctly handles HTTP 404 responses with a clear, actionable error message. The comment provides helpful context about branch-related scenarios, and the code follows the established pattern consistently.
test/e2e/compare.test.ts (6)
6-11: LGTM!The imports for
createBranchandcreateKeyare correctly added and used in the new Branching test suite.
39-39: LGTM!The describe label correctly reflects that this test suite uses
PROJECT_2data.
111-116: LGTM!The updated expectations correctly reflect the new output format with
+prefix for added keys and-prefix for removed keys.Also applies to: 135-140
160-179: LGTM!The Branching test setup is well-structured: creates project, PAK, feature branch, and adds keys to the branch. The
afterEachcleanup ensures proper teardown.
181-198: LGTM!Good test coverage - verifies that keys created on
feature-branchare detected as in-sync when explicitly targeting that branch via--branchoption.
200-210: LGTM!Correctly validates that branch-specific keys don't appear in the main branch, resulting in an "out of sync" status with 2 new keys reported.
test/e2e/utils/api/common.ts (3)
56-58: LGTM!The formatting change is minor and doesn't affect functionality.
127-145: LGTM!The
createKeyhelper is well-designed:
- Uses
Partial<Omit<...>>to make all fields optional exceptname- Correctly defaults
isPluraltofalseas required byCreateKeyDto- Returns the API response for callers who need it
147-157: LGTM!The expanded
DEFAULT_SCOPESinclude the necessary permissions (keys.create,keys.edit, etc.) for the new branch and key creation tests.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/commands/tag.ts (1)
47-64: Don’t always sendbranchin query; omit it when undefined (avoidsbranch=undefined/empty serialization risk).On Line 52, consider conditionally adding the query param (you already do this pattern in tests).
Proposed fix
const loadable = await loading( `Tagging${appendBranch(opts.branch)}...`, opts.client.PUT('/v2/projects/{projectId}/tag-complex', { params: { path: { projectId: opts.client.getProjectId() }, - query: { branch: opts.branch }, + query: { ...(opts.branch ? { branch: opts.branch } : {}) }, }, body: { filterTag: opts.filterTag, filterTagNot: opts.filterNoTag, tagFiltered: opts.tag, tagOther: opts.tagOther, untagFiltered: opts.untag, untagOther: opts.untagOther, filterKeys: opts.filterExtracted ? extractedKeys : undefined, filterKeysNot: opts.filterNotExtracted ? extractedKeys : undefined, }, }) );src/commands/sync/sync.ts (1)
187-197: Missing branch parameter in DELETE keys API call.Line 189 includes branch in the user message, but lines 190-194 don't pass the
branchquery parameter to the DELETE endpoint. This is inconsistent with the create flow (lines 169-172) which correctly includesbranchin the query.This means keys will be deleted from the wrong branch (default) when
opts.branchis specified.🔧 Proposed fix
const loadable = await loading( `Deleting unused keys${appendBranch(opts.branch)}...`, opts.client.DELETE('/v2/projects/{projectId}/keys', { - params: { path: { projectId: opts.client.getProjectId() } }, + params: { + path: { projectId: opts.client.getProjectId() }, + query: { branch: opts.branch }, + }, body: { ids }, }) );
🤖 Fix all issues with AI agents
In @src/commands/merge.ts:
- Around line 53-58: The buildMergeUrl function currently constructs frontend
URLs using opts.apiUrl which fails when API and frontend domains differ; update
the MergeOptions/BaseOptions types to include an optional frontendUrl and change
buildMergeUrl to prefer opts.frontendUrl (falling back to opts.apiUrl) when
building the URL, or alternatively add clear documentation on the limitation if
you choose not to update types; reference MergeOptions, BaseOptions, and
buildMergeUrl to locate where to add the new optional frontendUrl and use it
when composing the `/projects/${opts.projectId}/branches/merge/${mergeId}` URL.
In @test/e2e/pull.test.ts:
- Around line 435-451: The Branching test suite calls setupTemporaryFolder() but
never removes it; update the teardown to call and await removeTmpFolder() (or
removeTmpFolder in afterEach/afterAll alongside deleteProject) so TMP_FOLDER is
cleaned up; ensure removeTmpFolder is imported/available and placed in the same
afterEach block that currently calls deleteProject to guarantee the temp folder
is removed after each test.
🧹 Nitpick comments (3)
src/commands/tag.ts (1)
47-49: Loading message for “no branch” might be noisier than needed.On Line 48,
appendBranch(undefined)produces(none or default branch)(persrc/utils/branch.ts). If you want the default UX to remain “Tagging…”, consider makingappendBranchreturn''when no branch, and using a separate helper where you explicitly want to mention default branch.test/e2e/tags.test.ts (1)
258-307: Branching test: prefer asserting on a specific key instead of “exactly 1 key has tag” (less fixture-coupled).On Lines 279-284, asserting
.length === 1will break if the fixture gains another extracted key. If the intent is “controller is tagged in branch only”, assert on that key name directly.src/utils/branch.ts (1)
6-8: Clarify the message for the no-branch case.The message
" (none or default branch)"conflates two distinct states:
- No branch specified (none)
- Default branch is being used
This ambiguity could confuse users. Consider:
" (default branch)"if you can determine the default is being used" (no branch specified)"if branch context is absent- Or split the logic to show the actual default branch name when available
💡 Suggested improvement
-export function appendBranch(branch?: string) { - return branch ? ` (branch "${branch}")` : ' (none or default branch)'; -} +export function appendBranch(branch?: string) { + return branch ? ` (branch "${branch}")` : ' (default branch)'; +}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/cli.tssrc/client/internal/schema.generated.tssrc/commands/branch.tssrc/commands/merge.tssrc/commands/sync/sync.tssrc/commands/tag.tssrc/utils/branch.tstest/e2e/branch.test.tstest/e2e/merge.test.tstest/e2e/pull.test.tstest/e2e/sync.test.tstest/e2e/tags.test.tstest/e2e/utils/api/tags.ts
🧰 Additional context used
🧬 Code graph analysis (9)
src/commands/tag.ts (1)
src/utils/branch.ts (1)
appendBranch(6-8)
test/e2e/tags.test.ts (2)
test/e2e/utils/api/common.ts (1)
createBranch(100-125)test/e2e/utils/api/tags.ts (1)
getTagsMap(42-56)
src/commands/merge.ts (4)
src/options.ts (1)
BaseOptions(32-42)src/utils/logger.ts (4)
loading(125-155)error(69-71)info(42-44)success(51-53)src/utils/branch.ts (2)
fetchBranches(10-16)listBranches(18-33)src/client/TolgeeClient.ts (1)
handleLoadableError(22-26)
test/e2e/branch.test.ts (5)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)src/utils/branch.ts (1)
fetchBranches(10-16)test/e2e/utils/api/common.ts (5)
createProjectWithClient(49-94)createPak(159-165)DEFAULT_SCOPES(147-157)deleteProject(39-43)createBranch(100-125)test/e2e/utils/api/project1.ts (1)
PROJECT_1(6-15)test/e2e/utils/run.ts (1)
run(106-112)
test/e2e/utils/api/tags.ts (1)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)
src/cli.ts (3)
src/options.ts (1)
PROJECT_BRANCH(59-62)src/commands/branch.ts (1)
config(116-135)src/commands/merge.ts (1)
config(171-175)
test/e2e/merge.test.ts (3)
test/e2e/utils/api/common.ts (6)
createProjectWithClient(49-94)createPak(159-165)DEFAULT_SCOPES(147-157)deleteProject(39-43)createBranch(100-125)createKey(131-145)test/e2e/utils/api/project1.ts (1)
PROJECT_1(6-15)test/e2e/utils/run.ts (1)
run(106-112)
src/commands/sync/sync.ts (3)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)src/utils/logger.ts (1)
loading(125-155)src/utils/branch.ts (1)
appendBranch(6-8)
test/e2e/sync.test.ts (4)
test/e2e/utils/api/common.ts (5)
createProjectWithClient(49-94)createPak(159-165)createBranch(100-125)deleteProject(39-43)createKey(131-145)test/e2e/utils/api/project2.ts (1)
PROJECT_2(6-15)test/e2e/utils/tmp.ts (1)
removeTmpFolder(35-40)test/e2e/utils/run.ts (1)
run(106-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
🔇 Additional comments (20)
test/e2e/utils/api/tags.ts (1)
42-48: Branch-scoped tag fetching looks correct; nice conditional query merge.test/e2e/tags.test.ts (1)
53-65: Switch to--configusage is consistent and reads well.Also applies to: 84-85, 96-110, 130-131, 142-151, 167-168, 177-190, 209-210, 221-232, 241-250
test/e2e/sync.test.ts (2)
13-20: Branch helpers import integration is straightforward.
290-392: Branching coverage is solid (branch-scoped keys + missing branch).test/e2e/pull.test.ts (2)
13-19: Branch helpers import integration is straightforward.
452-495: Branch pull scenarios (include/exclude + missing branch) are well targeted.src/cli.ts (1)
22-22: LGTM! Clean integration of branch functionality.The branch option and commands are properly wired into the CLI:
PROJECT_BRANCHoption correctly imported and registered with config defaultBranchCommandandMergeCommandfollow the established pattern for command registration- Global options are properly exposed for both commands
Also applies to: 40-41, 186-186, 216-221
src/utils/branch.ts (1)
10-16: LGTM! Branch utilities are well-structured.Both
fetchBranchesandlistBranchesare cleanly implemented:
- Proper error handling via
handleLoadableError- Clear branch markers (default, protected, inactive, ongoing merge)
- Good user feedback
Also applies to: 18-33
test/e2e/merge.test.ts (1)
16-24: LGTM! Comprehensive e2e test coverage.The test suite effectively covers both merge scenarios:
- Success case: Verifies clean merge completes with exit code 0 and correct output
- Conflict case: Ensures conflicts are detected, reported, and prevent merge completion
- Proper setup/teardown with project lifecycle management
- Good assertions on both stdout/stderr and API state
Also applies to: 26-46, 48-74
src/commands/sync/sync.ts (1)
22-22: LGTM! Branch context properly integrated into sync flow.The branch parameter is correctly threaded through:
- Backup export with
filterBranch- All-keys fetch with
branchquery param- Key creation/import with
branchquery param- User-facing messages with
appendBranchsuffixAlso applies to: 33-48, 96-104, 146-148, 167-175
src/commands/merge.ts (1)
18-29: LGTM! Well-structured merge implementation.The merge command is cleanly implemented with:
- Good validation: Mutual exclusivity check for branch argument vs option (lines 19-23)
- Clear user feedback: Change rendering with symbols (+, ~, -, x) and language tags
- Proper error handling: Guards against non-existent and default branches
- Conflict detection: Robust logic with fallback from API count to client-side filtering (lines 137-142)
- Complete workflow: Preview → refresh → check → apply with loading indicators at each step
Also applies to: 31-51, 60-169
src/commands/branch.ts (5)
1-13: LGTM! Clean imports and type definitions.The imports are well-organized, and the
BranchOptionstype definition properly extendsBaseOptionswith the necessary branch-specific options.
15-32: LGTM! Proper validation of mutual exclusivity.The function correctly enforces that the positional
[branch]argument cannot be used together with--createor--deleteoptions. Error messages are clear and guide users to the correct usage.
34-59: LGTM! Solid origin resolution with helpful error handling.The function properly resolves the origin branch, falling back to the project's default branch when no origin is specified. The inclusion of
listBranches()in error paths provides helpful feedback to users.
61-114: LGTM! Well-structured command handler with proper error handling.The handler cleanly separates the list, create, and delete flows. Good use of:
- Loading indicators for better UX
handleLoadableErrorfor consistent error handling- Clear success messages
- Branch existence validation before deletion
116-135: LGTM! Proper commander configuration with mutual exclusivity.The command is well-configured with:
- Clear description and argument definition
- Proper use of
conflicts()for mutually exclusive options- Optional positional argument that supports the list-by-default behavior
One thing to verify: Branch management operations require the
project.editscope (as seen in the test file). Ensure this requirement is documented in user-facing documentation or help text.test/e2e/branch.test.ts (4)
1-26: LGTM! Well-organized test setup with clean helpers.The test helpers (
fetchBranchesandfindBranch) provide a clean abstraction for test assertions. WhilefetchBranchesduplicates logic fromsrc/utils/branch.ts, this is acceptable for test isolation—tests should verify behavior via direct API calls rather than relying on implementation details.
28-82: LGTM! Comprehensive success path testing.The tests properly cover:
- Project setup with correct scopes (
project.editrequired for branch operations)- Clean teardown to avoid test pollution
- Listing branches with expected output format
- Creating branches via positional argument
- Creating branches with explicit origin and verifying the fork relationship
Each test validates both CLI output and actual API state, ensuring end-to-end correctness.
84-138: LGTM! Thorough error handling validation.The tests properly verify error scenarios:
- Branch deletion workflow with API verification
- Missing origin branch error with helpful error messages
- Missing target branch error with guidance
- All error cases check for non-zero exit codes and include branch listings to aid users
The pattern of combining
stderr + stdoutfor output assertions is a good practice for CLI testing.
140-170: LGTM! Complete validation of mutual exclusivity constraints.These tests verify that the CLI properly enforces mutual exclusivity between the positional
[branch]argument and the--create/--deleteoptions. The error messages are validated to ensure they provide clear guidance on correct usage.
zomp
force-pushed
the
janmolnar/branching
branch
from
7a501ec to
e27827b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/commands/merge.ts:
- Around line 53-58: The buildMergeUrl function can produce a URL pointing at
opts.apiUrl which may be the wrong host for frontend links; update MergeOptions
to accept an explicit webUrl (e.g., add webUrl?: string) and change
buildMergeUrl to use opts.webUrl when present, falling back to opts.apiUrl only
if webUrl is undefined; also update any help text or docs to mention the new
webUrl option so users know to configure a separate frontend URL when API and
web hosts differ.
🧹 Nitpick comments (1)
src/commands/merge.ts (1)
125-148: LGTM with a note on complexity.The change display and conflict detection logic is correct. The conflict count resolution (lines 137-141) uses a reasonable fallback strategy—prioritizing the server-provided count, then falling back to client-side counting. The logic is sound, though somewhat complex.
Optional: Extract conflict detection for clarity
Consider extracting the conflict count logic into a helper function to improve readability:
function getUnresolvedConflictCount( refreshData: any, changes: MergeChange[] ): number { return ( refreshData?.keyUnresolvedConflictsCount ?? changes.filter( (change) => change.type === 'CONFLICT' && !change.resolution ).length ); }Then use:
if (getUnresolvedConflictCount(refreshLoadable.data, changes) > 0) { ... }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/cli.tssrc/commands/branch.tssrc/commands/merge.tssrc/utils/branch.tstest/e2e/merge.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/branch.ts
- test/e2e/merge.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/cli.ts (3)
src/options.ts (1)
PROJECT_BRANCH(59-62)src/commands/branch.ts (1)
config(116-135)src/commands/merge.ts (1)
config(171-175)
src/commands/merge.ts (4)
src/options.ts (1)
BaseOptions(32-42)src/utils/logger.ts (5)
exitWithError(103-116)loading(125-155)error(69-71)info(42-44)success(51-53)src/utils/branch.ts (2)
fetchBranches(10-16)listBranches(18-33)src/client/TolgeeClient.ts (1)
handleLoadableError(22-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
🔇 Additional comments (16)
src/cli.ts (4)
22-22: LGTM!The import of
PROJECT_BRANCHis correctly placed and follows the existing pattern for importing options.
40-41: LGTM!The command imports are correctly placed and follow the existing pattern for importing command modules.
186-186: LGTM!The
PROJECT_BRANCHoption is correctly wired with a default from the configuration, following the same pattern as other optional settings.
216-221: LGTM!The command registrations for
BranchCommandandMergeCommandcorrectly follow the established pattern used for other commands in the CLI.src/commands/merge.ts (7)
1-16: LGTM!Imports and type definitions are clean and follow the existing patterns in the codebase.
18-29: LGTM!The branch name resolution logic correctly enforces mutual exclusivity and provides clear error messages.
31-51: LGTM!The rendering functions provide clear, intuitive representations of merge changes with appropriate fallback handling.
60-80: LGTM!The initial validation logic is thorough, with good error messages and helpful feedback (branch listing) when validation fails.
82-123: LGTM!The merge preparation and change fetching logic follows the established API call patterns with appropriate error handling and user feedback.
150-169: LGTM!The merge application and success reporting follow the established patterns and provide clear feedback to the user.
171-175: LGTM!The command export follows the standard pattern established in the codebase.
src/commands/branch.ts (5)
1-13: LGTM!Imports and type definitions are clean and appropriate for the command's functionality.
15-32: LGTM!The target name resolution correctly enforces mutual exclusivity constraints with clear error messages. The comment appropriately acknowledges Commander's built-in conflict handling.
34-59: LGTM!The origin branch resolution logic is solid, with appropriate validation and a sensible fallback to the default branch.
61-114: LGTM!The branch handler provides a clean, well-structured implementation with proper validation, error handling, and user feedback for both create and delete operations.
116-135: LGTM!The command definition is well-structured, making good use of Commander's built-in features for option conflicts and providing clear descriptions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/commands/sync/sync.ts:
- Around line 189-194: The DELETE call is placing the branch query at the top
level of the request options instead of inside params, so the branch is ignored;
update the opts.client.DELETE invocation to nest the branch under params (same
shape as the GET/POST requests) — i.e. keep params: { path: { projectId:
opts.client.getProjectId() }, ...(!!opts.branch && { query: { branch:
opts.branch } }) } alongside body: { ids } in the
opts.client.DELETE('/v2/projects/{projectId}/keys', ...) call (referencing
opts.client.DELETE, opts.branch and ids).
🧹 Nitpick comments (1)
src/commands/sync/compare.ts (1)
29-37: Nice integration of branch support with good UX!The loading wrapper and branch-aware message enhance the user experience. The conditional branch parameter works correctly.
The spread syntax
...(!!opts.branch && { query: { branch: opts.branch } })is functional but could be more readable. Consider a more conventional alternative:♻️ Optional readability improvement
const loadable = await loading( `Fetching Tolgee keys${appendBranch(opts.branch)}...`, opts.client.GET('/v2/projects/{projectId}/all-keys', { params: { path: { projectId: opts.client.getProjectId() }, - ...(!!opts.branch && { query: { branch: opts.branch } }), + ...(opts.branch ? { query: { branch: opts.branch } } : {}), }, }) );This makes the conditional logic more explicit without relying on spreading falsy values.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands/sync/compare.tssrc/commands/sync/sync.tssrc/commands/tag.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/tag.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands/sync/compare.ts (2)
src/utils/logger.ts (1)
loading(125-155)src/utils/branch.ts (1)
appendBranch(6-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
🔇 Additional comments (6)
src/commands/sync/compare.ts (1)
14-14: LGTM!The import of
appendBranchis clean and supports the branch-aware messaging enhancement.src/commands/sync/sync.ts (5)
22-22: LGTM!The import of
appendBranchutility is appropriate for adding branch context to user-facing messages.
33-40: LGTM!The
backupfunction signature correctly extends to include an optionalbranchparameter, and the parameter is properly passed to the export call asfilterBranch.
96-104: LGTM!The GET request correctly includes branch context in both the loading message and query parameters. The conditional spreading ensures the query parameter is only added when a branch is provided.
146-147: LGTM!The backup invocation correctly passes the branch parameter and includes branch context in the loading message.
167-174: LGTM!The POST request correctly includes branch context in both the loading message and query parameters, with proper structure matching the GET request pattern.
zomp
force-pushed
the
janmolnar/branching
branch
from
4cfed9d to
606f9fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/sync/sync.ts (1)
188-194: Branch parameter missing from DELETE operation.The loading message indicates branch-aware deletion via
appendBranch(opts.branch), but the DELETE API call doesn't include the branch as a query parameter. This is inconsistent with the GET (lines 96-103) and POST (lines 166-174) operations, which both conditionally pass the branch.This inconsistency could lead to keys being deleted from the wrong branch or unexpected API behavior.
🔧 Proposed fix
const loadable = await loading( `Deleting unused keys${appendBranch(opts.branch)}...`, opts.client.DELETE('/v2/projects/{projectId}/keys', { - params: { path: { projectId: opts.client.getProjectId() } }, + params: { + path: { projectId: opts.client.getProjectId() }, + ...(!!opts.branch && { query: { branch: opts.branch } }), + }, body: { ids }, }) );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands/sync/compare.tssrc/commands/sync/sync.tssrc/commands/tag.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/commands/tag.ts (1)
src/utils/branch.ts (1)
appendBranch(6-8)
src/commands/sync/compare.ts (2)
src/utils/logger.ts (1)
loading(125-155)src/utils/branch.ts (1)
appendBranch(6-8)
src/commands/sync/sync.ts (3)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)src/utils/logger.ts (1)
loading(125-155)src/utils/branch.ts (1)
appendBranch(6-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: Unit Tests (windows-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
🔇 Additional comments (6)
src/commands/tag.ts (1)
47-65: LGTM! Branch support correctly integrated.The branch parameter is properly threaded through the tagging operation:
- Loading message reflects the branch context via
appendBranch- API call conditionally includes branch as a query parameter
- Implementation is clean and consistent
src/commands/sync/compare.ts (1)
29-37: LGTM! Branch awareness correctly implemented.The compare flow now supports branch-specific key fetching with appropriate user feedback.
src/commands/sync/sync.ts (4)
33-48: LGTM! Backup function correctly extended for branch support.The function signature change is minimal and the branch is properly passed to the export operation via
filterBranch.
96-104: LGTM! Branch support correctly added to key fetching.
145-148: LGTM! Backup invocation correctly passes branch.
166-175: LGTM! Key creation correctly includes branch context.
Summary by CodeRabbit
New Features
Validation
UX
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.